feat: add memory profiling support #39
Conversation
not-matthias
changed the title
Merging this PR will degrade performance by 39.4%
Performance Changes
Comparing 
Footnotes
|
not-matthias
force-pushed
the
cod-1945-support-memory-profiling-for-c
branch
from
7b0a7b4 to
1b97510
Compare
When running the warmup, we already call `start/stop_benchmark` which creates two memory profiles per benchmark, which breaks our assumptions in the backend.
not-matthias
force-pushed
the
cod-1945-support-memory-profiling-for-c
branch
from
b158911 to
003a1bf
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds memory profiling support to the benchmarking framework by introducing a new "memory" mode alongside the existing "simulation" and "walltime" modes. The changes unify the preprocessor directives and add the necessary configuration and infrastructure for memory profiling.
Changes:
- Renamed
CODSPEED_SIMULATIONpreprocessor macro toCODSPEED_ANALYSISto better represent both simulation and memory profiling modes - Added "memory" as a new valid mode in CMake and Bazel build configurations
- Removed warmup execution logic from simulation mode to reduce flakiness
- Added memory benchmark examples demonstrating various allocation patterns
- Updated CI workflows to test the new memory profiling mode with memtrack CLI installation
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| google_benchmark/src/benchmark_runner.cc | Updated preprocessor directive from CODSPEED_SIMULATION to CODSPEED_ANALYSIS |
| google_benchmark/src/benchmark_api_internal.h | Updated preprocessor directives and removed warmup execution logic |
| google_benchmark/src/benchmark_api_internal.cc | Removed warmup repetition code from simulation mode |
| google_benchmark/src/benchmark.cc | Updated preprocessor directives and console output message |
| google_benchmark/include/benchmark/benchmark.h | Updated preprocessor directives throughout the State class |
| examples/google_benchmark_cmake/memory_bench.hpp | Added new memory profiling benchmark examples with RLE encoding/decoding and allocation patterns |
| examples/google_benchmark_cmake/main.cpp | Added include for new memory benchmark header |
| examples/google_benchmark_bazel/memory_bench.hpp | Added symlink to cmake memory benchmark file |
| core/instrument-hooks | Updated submodule commit reference |
| core/CMakeLists.txt | Added "memory" to allowed modes and updated preprocessor definitions |
| core/BUILD | Added memory_mode configuration and updated preprocessor defines |
| .github/workflows/ci.yml | Added memory mode to test matrix and memtrack CLI installation steps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #ifdef CODSPEED_ANALYSIS | ||
| State BenchmarkInstance::RunSimulation( | ||
| codspeed::CodSpeed* codspeed, internal::ThreadTimer* timer, | ||
| internal::ThreadManager* manager, | ||
| internal::PerfCountersMeasurement* perf_counters_measurement, | ||
| ProfilerManager* profiler_manager) const { |
There was a problem hiding this comment.
The function name 'RunSimulation' is now misleading since CODSPEED_ANALYSIS covers both simulation and memory modes. Consider renaming to 'RunAnalysis' or a more generic name that reflects its broader purpose.
| // Determine the width of the name field using a minimum width of 10. | ||
| #ifdef CODSPEED_SIMULATION | ||
| #ifdef CODSPEED_ANALYSIS | ||
| Err << "Codspeed mode: simulation" << "\n"; |
There was a problem hiding this comment.
The error message still says 'simulation' but this code path is now used for both simulation and memory modes under CODSPEED_ANALYSIS. The message should differentiate between these modes or be more generic.
| Err << "Codspeed mode: simulation" << "\n"; | |
| Err << "Codspeed mode: analysis" << "\n"; |
There was a problem hiding this comment.
Not sure if printing analysis is the best UX, since we said it should be only the internal build mode. I guess we could forward the real measurement mode and then print that, while also enabling CODSPEED_WALLTIME/CODSPEED_ANALYSIS. wdyt?
TODO: REmove the custom memtrack installation before merging